(3/N) db_metadata_nexus queries #8931
Conversation
smklein
changed the title
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
e301683 to
b3e696e
Compare
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
b3e696e to
20fafc3
Compare
smklein
force-pushed
the
db_metadata_nexus_records
branch
from
6e20a24 to
23a3335
Compare
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
20fafc3 to
b884dfa
Compare
smklein
changed the base branch from
db_metadata_nexus_records
to
nexus_gen_schema
smklein
changed the title
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
b884dfa to
a91fb35
Compare
smklein
force-pushed
the
nexus_gen_schema
branch
from
dbcfe45 to
c4df197
Compare
smklein
force-pushed
the
nexus_gen_schema
branch
from
c4df197 to
345009a
Compare
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
a91fb35 to
0a20282
Compare
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
0a20282 to
8e93726
Compare
smklein
force-pushed
the
db_metadata_nexus_queries
branch
from
8e93726 to
2dba0e1
Compare
| /// Describes the state of the database access with respect this Nexus | ||
| #[derive(Debug, Copy, Clone, PartialEq)] | ||
| enum NexusAccess { | ||
| /// Nexus does not yet have access to the database. |
There was a problem hiding this comment.
| /// Nexus does not yet have access to the database. | |
| /// Nexus does not yet have access to the database, but can take over when current-generation Nexus instances quiesce |
There was a problem hiding this comment.
Updated in 6034c51
| /// Nexus does not yet have access to the database. | ||
| DoesNotHaveAccessYet { nexus_id: OmicronZoneUuid }, | ||
|
|
||
| /// Nexus has been explicitly locked out of the database. |
There was a problem hiding this comment.
| /// Nexus has been explicitly locked out of the database. | |
| /// Nexus has been permanently, explicitly locked out of the database. |
There was a problem hiding this comment.
Updated in 6034c51
| /// Start a schema update | ||
| Update, | ||
|
|
||
| /// Refuse to use the database |
There was a problem hiding this comment.
| /// Refuse to use the database | |
| /// Permanently refuse to use the database |
There was a problem hiding this comment.
Updated in 6034c51
| // - Systems that haven't been migrated to include nexus access control | ||
| // (we need access to the database to backfill these records). |
There was a problem hiding this comment.
I think this case isn't necessary because of the schema migration.
There was a problem hiding this comment.
I think in an older version, this check was taken even for the schema-updater binary, but that's no longer the case with the IdentityCheckPolicy::DontCare option.
Updated in 6034c51
| return Ok(NexusAccess::HasImplicitAccess); | ||
| } | ||
|
|
||
| // Records exist, so enforce the access control check |
There was a problem hiding this comment.
| // Records exist, so enforce the access control check | |
| // Records exist, so enforce the identity check |
We didn't really talk about this but I've been starting to use the term "identity check" rather than "access control" to avoid confusing it with IAM/RBAC/authz sort of stuff. What do you think?
There was a problem hiding this comment.
Sounds good to me; definitely reasonable to avoid "access control" as a term because that's overloaded.
Updated in 6034c51
| let msg = "Nexus does not have access to the database (no \ | ||
| db_metadata_nexus record)"; | ||
| warn!(&self.log, "{msg}"; "nexus_id" => ?nexus_id); | ||
| return Ok(NexusAccess::DoesNotHaveAccessYet { nexus_id }); |
There was a problem hiding this comment.
I think this needs to be a different variant because right now it's indistinguishable from the case of finding a NotYet record (L287). It looks to me like we'll wind up creating a DatastoreSetupAction::NeedsHandoff, which is not correct for this case. I believe the correct answer is to wait a bit and check the whole thing again.
| enum NexusAccess { | ||
| /// Nexus does not yet have access to the database, but can take over when | ||
| /// the current-generation Nexus instances quiesce. | ||
| DoesNotHaveAccessYet { nexus_id: OmicronZoneUuid }, |
There was a problem hiding this comment.
Is this our own nexus_id? Why do we carry it in this variant and not the others?
There was a problem hiding this comment.
This is a follow-up from #8845 (comment)
In short: NexusAccess::DoesNotHaveAccessYet can result in us returning DatastoreSetupAction::NeedsHandoff to a user. In this case, they must have a Nexus ID to use to initiate handoff.
There was an error case I previously handled at runtime, to the effect of "if a client calls check_schema_and_access, and gets back the NeedsHandoff action, but does not have a Nexus ID, then throw an error". But this case is impossible - we only return that result when an explicit Nexus ID is passed through IdentityCheckPolicy -> NexusAccess -> DatastoreSetupAction.
Rather than having an impossible-to-cover conditional, I just passed the Nexus UUID through the endpoint that needs it, for the purposes of feeding-forward to the output.
Split off of #8845
Adds and tests queries which will be used in integration (reading on boot): #8925
Does not actually flip Nexus to use these records yet.
Depends on #8924
Next part of #8501: Adding queries for these records